feat(react-router): support react router v8#21633
Conversation
JPeer264
requested review from
Lms24,
chargome,
nicohrubec and
s1gr1d
and removed request for
a team
nicohrubec
left a comment
nicohrubec
left a comment
There was a problem hiding this comment.
as discussed offline maybe we could simplify this so we don't have to ship one integration per major and instead use one generic one
| ], | ||
| }, | ||
| // todo: should be 'GET /errors/server-loader' | ||
| transaction: 'GET /{*splat}', |
There was a problem hiding this comment.
Just as a reference, because commit comments are not shown in PRs: b084c55#r189222849
| "forceConsistentCasingInFileNames": true, | ||
| "noFallthroughCasesInSwitch": true, | ||
| "module": "esnext", | ||
| "moduleResolution": "bundler", |
There was a problem hiding this comment.
Just as a reference, because commit comments are not shown in PRs: b084c55#r189222251
JPeer264
force-pushed
the
jp/support-react-router-8
branch
from
84142b2 to
feb5a0d
Compare
As a written update. Now the version additions in the The |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 694f162. Configure here.
| const user: User = await getUser(); | ||
| context.set(userContext, user); | ||
| await next(); | ||
| }); |
There was a problem hiding this comment.
Middleware omits awaiting startSpan
Medium Severity
The authMiddleware handler invokes Sentry.startSpan without return or await, so the middleware promise can settle before the span callback runs await next(). That breaks the usual React Router middleware contract and can race the loader against context setup or tracing.
Reviewed by Cursor Bugbot for commit 694f162. Configure here.
| }); | ||
|
|
||
| await page.goto(`/performance`); // pageload | ||
| await page.waitForTimeout(1000); // give it a sec before navigation |
There was a problem hiding this comment.
E2E tests use fixed sleeps
Low Severity
Several new React Router 8 framework performance tests call page.waitForTimeout(1000) before navigation instead of waiting on a concrete telemetry or UI signal. Fixed sleeps are a common source of flaky CI when load timing varies.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 694f162. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| * Works with React Router v6+. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| export function wrapReactRouterRouting<P extends Record<string, any>, R extends React.FC<P>>(routes: R): R { |
There was a problem hiding this comment.
Bug: The file uses React.FC in its type definitions without explicitly importing the React namespace, creating an implicit dependency and code inconsistency.
Severity: LOW
Suggested Fix
Add import * as React from 'react'; to the top of packages/react/src/reactrouter.compat.tsx to make the dependency on the React namespace explicit and improve code consistency and maintainability.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/react/src/reactrouter.compat.tsx#L32
Potential issue: The file `reactrouter.compat.tsx` uses the `React.FC` type in its
definitions without an explicit `import * as React from 'react';` statement. While this
may compile successfully due to TypeScript resolving `React` as a global type from
ambient declarations, it creates an implicit dependency on the toolchain's
configuration. This is inconsistent with other files like `reactrouter.tsx` which do
import React, making the code less maintainable and potentially brittle to future build
system changes.
Did we get this right? 👍 / 👎 to inform future reviews.
2 participants
closes #21622
closes JS-2800
Basically only
packages/react/src/reactrouterv8.tsxhas been added to make it work. I copied over 3 tests:-cross-usage,-spaand-frameworkthat covers the exports.To make it easier to check what actually changed in the apps, I made one commit with the copy and another with the changes. So it might be easier to review only the actual changes: b084c55
Docs will be updated once the PR lands